Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1104-flatlist-gesture-handling-enhancement #120

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LunatiqueCoder
Copy link
Owner

Fixes #104

@LunatiqueCoder LunatiqueCoder linked an issue Aug 10, 2024 that may be closed by this pull request
@coofzilla
Copy link

coofzilla commented Aug 12, 2024

okay so, I tested this and just had a question, will I have to trigger the disable scroll myself? so this isn't going to be handled internally?

here is a quick demo of how I tested it.

const videoData = [
  {id: '1', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '2', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '3', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '4', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
];

export default function HomeScreen() {
  const flatListRef = useRef<FlatList>(null);
  const [scrollEnabled, setScrollEnabled] = useState<boolean>(true);

  const handleScrollEnabled = (enabled: boolean) => {
    setScrollEnabled(enabled);
  };

  return (
    <>
      <FlatList
        ref={flatListRef}
        scrollEnabled={scrollEnabled}
        data={videoData}
        keyExtractor={(item) => item.id}
        horizontal
        renderItem={({item}) => (
          <View style={styles.videoContainer}>
            <VideoPlayer
              pan={{
                parentList: {
                  ref: flatListRef,
                  scrollEnabled: scrollEnabled,
                },
              }}
              source={{uri: item.uri}}
              onSeek={() => {
                console.log('Seeking');
              }}
              muted={true}
            />
          </View>
        )}
      />
      <Button
        title={scrollEnabled ? 'Disable Scroll' : 'Enable Scroll'}
        onPress={() => handleScrollEnabled(!scrollEnabled)}
      />
    </>
  );
}

This works as in, I can disable scrolling and then seek/scroll the volume, just wondering if this is the intended flow or, if I'm missing something so that its abstracted away?

@coofzilla
Copy link

In other words, am I going to have to manually trigger the disable scroll, and then seek adjust the volume, then re-enable it when I want to go back to scrolling? 🤔 My apologies if this is obvious and I'm just missing something.

@LunatiqueCoder
Copy link
Owner Author

LunatiqueCoder commented Aug 12, 2024

@coofzilla You don't need need to manually trigger the disable scroll. This should happen internally. Although, there might be some certain scenarios where the <FlatList /> needs to use scrollEnabled prop, and the videoplayer has to be aware of it to revert back to the value it was before seeking.

📖 Explanation:

If FlatList's scrollEnabled={false}, the videoplayer needs to know,
else it will use setNativeProps({scrollEnabled: true}) after seeking is finished, leading to a huge inconsistency between the scrollEnabled state and the native props. (one if them is true, the other is false)

setNativeProps is imperative and stores state in the native layer (DOM, UIView, etc.) and not within your React components, which makes your code more difficult to reason about.

If you update a property that is also managed by the render function, you might end up with some unpredictable and confusing bugs because anytime the component re-renders and that property changes, whatever value was previously set from setNativeProps will be completely ignored and overridden.

More info here: https://reactnative.dev/docs/direct-manipulation



But as a short answer, if you don't use the scrollEnabled prop on the <FlatList />, you're good to go just by passing the flatlist ref. However, if you use scrollEnabled prop, you also have to pass the same value/state to the videoplayer in order to avoid bugs.

This works as in, I can disable scrolling and then seek/scroll the volume, just wondering if this is the intended flow or, if I'm missing something so that its abstracted away?

If you have to disable the scrolling first, then there's definitely an issue with this implementation. However it seems to be working on my side 🤔

I really hope this is not that confusing, I also found it very hard to describe it in the docs. Let me know if this answers your questions, or I can try to be more precise when explaining.

@LunatiqueCoder
Copy link
Owner Author

@coofzilla

This works as in, I can disable scrolling and then seek/scroll the volume, just wondering if this is the intended flow or, if I'm missing something so that its abstracted away?

Yeah, I could find an issue with what you described here. I'll try to fix it tomorrow. Thank you for your help! Really appreciate it!

@coofzilla
Copy link

hey thanks for the explanation dude! Glad you were able to see the issue I was describing, I really appreciate it. Let me know when I can test further 🫡

@LunatiqueCoder
Copy link
Owner Author

@coofzilla You can try the new changes now. Hopefully it's fine now. 🙏

@coofzilla
Copy link

coofzilla commented Aug 13, 2024

Do you have a snippet that I can try? Its still scrolling when I try to seek/change the volume. ( I pulled the most recent changes )

demo_scroll_conflict.mov
const videoData = [
  {id: '1', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '2', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '3', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
  {id: '4', uri: 'https://vjs.zencdn.net/v/oceans.mp4'},
];

const screenWidth = Dimensions.get('window').width;

export default function HomeScreen() {
  const flatListRef = useRef<FlatList>(null);

  return (
    <View style={{flex: 1, width: screenWidth}}>
      <FlatList
        ref={flatListRef}
        data={videoData}
        keyExtractor={(item) => item.id}
        horizontal
        pagingEnabled
        renderItem={({item}) => (
          <View style={styles.videoContainer}>
            <VideoPlayer
              pan={{
                parentList: {
                  ref: flatListRef,
                },
              }}
              source={{uri: item.uri}}
              muted={true}
            />
          </View>
        )}
      />
    </View>
  );
}

const styles = StyleSheet.create({
  videoContainer: {
    width: screenWidth,
  },
});

@LunatiqueCoder
Copy link
Owner Author

@coofzilla Your code snippet works perfectly fine for me now 🤔 You might need to run yarn prepare inside ./packages/media-console folder. This last commit was the fix, be sure it's also there in the code: 75aafc8

@coofzilla
Copy link

Alrighty, I'll get that checked out probably tomorrow and report back. Sorry, I have to put out some fires. 🧯 Thank you again for the help with this!

@coofzilla
Copy link

coofzilla commented Aug 13, 2024

LETS GO! Works Perfect!!! I didn't have the last commit 🤦

demo.mov

@LunatiqueCoder LunatiqueCoder self-assigned this Aug 13, 2024
@LunatiqueCoder LunatiqueCoder added the bug Something isn't working label Aug 13, 2024
@LunatiqueCoder
Copy link
Owner Author

@coofzilla Really happy it works for you as well. I still need to add it to the docs and think how to describe it so it won't create confusion while using it. Thanks for the help!

@coofzilla
Copy link

@coofzilla Really happy it works for you as well. I still need to add it to the docs and think how to describe it so it won't create confusion while using it. Thanks for the help!

Given the example app, I was thinking what if there was a page with like "examples" or "use cases" as a list (each item being a different case that would press and go to the example) that could just scale as various ways to implement it are thought of. Or when people want to experiment/contribute, they could like, add theirs to the list. kinda like what this repo does.

Not as a replacement for docs; but, as a supplement?

This way when someone is like ahh, I really wish I knew how to implement this as a horizontal list and didn't have the conflicting scrolling, boom, you just have it as

1. Horizontal List
2. Vertical Feed
3. Paginated List
4. Custom Icons 🫣

here is a quick demo:
image
image

Maybe thats too much for the purpose of this library haha; but, just a thought :)

@LunatiqueCoder
Copy link
Owner Author

LunatiqueCoder commented Aug 13, 2024

@coofzilla Actually, that's a good idea. Besides being easier to contribute and test the changes, that's the purpose of the example app. I am planning to add various examples that users can just copy paste, however, I want to take care of the critical issues first.

Nevertheless PRs are welcome :) Active contributors are eligible to receive a license for all JetBrains IDEs: https://www.jetbrains.com/ides/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Gesture Handling Enhancement for use with FlatList
3 participants